-
-
Notifications
You must be signed in to change notification settings - Fork 878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: update typescript to 5.4.3 #2404
Conversation
Hi @erikbrinkman, EP suggested that you might be able to offer some guidance on this TypeScript error. No pressure though of course. This is only after upgrading to
The error seems to be referring to this line, but I can't fix it with any combination of adding the
Whichever you put, however, you get either
Update 03/04/24: I managed to get everything working with one tiny change. However I don't have enough context to understand if this change is right or not. By changing the type to allow |
@epoberezkin fyi |
Sorry for the delay here. Your fix is not correct, but I applaud tracking down something that works from these outrageous messages. What you changed means if a type is undefined, you no longer need to mark it as nullable, but that's not correct, if a type is undefined, than the schema should indeed have As far as the root cause, I'm still tracking it down. I find the typescript playground is helpful for figuring these things out. The problem isn't in directly checking the definitions, it's actually in the way we define const _: SomeJSONSchema = {
// ... and here ...
type: "array",
items: [{ type: "number" }, { type: "string", nullable: true }],
minItems: 2,
additionalItems: false,
} type checks fine, but if you remove the nullable for the string item, it fails. As far as I can tell, this traces back to how we define: type SomeJSONSchema = UncheckedJSONSchemaType<Known, true>
type Known = {
key: string]: Known;
} | [Known, ...Known[]] | Known[] | number | string | boolean | null particularly the tuple definition I need to understand better how typescript interprets this tuple expansion, but I think it went from being essentially a heterogeneous tuple, to being a tuple with one element, and then a homogenous array, which screws up the inference. I think realistically the defintion of |
Hmm, interesting. In the meantime @erikbrinkman what about this bump instead? Not sure if you're able to review PRs, but feel free to give it a tick if you think this is ok. |
Closing for now as we have an update to 5.3.3 without any changes needed. |
What issue does this pull request resolve?
What changes did you make?
Is there anything that requires more attention while reviewing?
The change the the
Nullable
type.